WIP: dedicated pool dialers to vtorc and to throttler functions, no change in "concurrency"
#15560
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
shlomi-noach
added
Type: Bug
Component: VTorc
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
Signed-off-by: Shlomi Noach <[email protected]>
Comment on lines
+106
to
+116
| // rpcClientMap maps an address to a tmc | ||
| type rpcClientMap map[string](chan *tmc) | ||
|
|
||
| // grpcClient implements both dialer and poolDialer. | ||
| type grpcClient struct { | ||
| // This cache of connections is to maximize QPS for ExecuteFetchAs{Dba,App}, | ||
| // CheckThrottler and FullStatus. Note we'll keep the clients open and close them upon Close() only. | ||
| // But that's OK because usually the tasks that use them are one-purpose only. | ||
| // The map is protected by the mutex. | ||
| mu sync.Mutex | ||
| rpcClientMap map[string]chan *tmc | ||
| mu sync.Mutex | ||
| rpcClientMaps map[DialPoolGroup]rpcClientMap |
There was a problem hiding this comment.
DialPoolGroupThrottler and DialPoolGroupVTOrc do not need a channel and should only open One connection per tablet address.
better to have a different type for them and leave the other concurrent one seperate.
There was a problem hiding this comment.
Is it OK if I still make the code idential for all of them, but use channel size 1 for VTOrc and Throttler?
There was a problem hiding this comment.
See proposed solution in a2dd6fe
There was a problem hiding this comment.
CI doesn't like a2dd6fe
Signed-off-by: Shlomi Noach <[email protected]>
Closed
5 tasks
|
Yeah I messed up something here. I'm gonna create yet another final draft PR, where I keep all the existing logic, and add bespoke logic for the non-default dial groups (vtorc, throttler). |
5 tasks
|
Closing in favor of #15562 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
One final alternative, this is similar to #15559, but while #15559 remvoes the so-called "concurrency" element, this PR preserves it. In this PR:
vtorc; another, dedicated to the tablet throttler.dialPoolfunction returns aninvalidator, much like Fix for FullStatus gRPC connection pooling #15520. However, onlyFullStatusandCheckThrottlerchoose to invoke it. When they do, they do not affect any other pool dialer.Related Issue(s)
#15563
Checklist
Deployment Notes